Repository navigation
Skip assignments when narrowing identifiers - #64878
Jake Bailey (jakebailey) wants to merge 2 commits into
Conversation
Narrowing an identifier walks back through every assignment on its
flow path. In long straight-line functions each reference walks past
every earlier declaration, so the walk length grows with the function
and most of the time goes to cache misses loading each declaration
and its name, only to find a different name.
The binder now records a hash of the identifier each assignment flow
node can target, in what was padding in FlowNode. Walks for an
identifier skip assignments whose recorded name differs without
touching their syntax. Assignments that could matter for any
reference, like for-in variables, are recorded as unknown and still
checked in full.
--checkers 4, n=8:
│ before │ after │
│ sec/op │ sec/op vs base │
Tsc/bluesky 2.794 ± 3% 2.757 ± 2% ~ (p=0.195 n=8)
Tsc/sentry 11.02 ± 2% 11.17 ± 3% ~ (p=0.505 n=8)
Tsc/t3code-server 13.10 ± 2% 12.04 ± 2% -8.14% (p=0.000 n=8)
geomean 7.388 7.184 -2.77%
│ before │ after │
│ rss-B │ rss-B vs base │
Tsc/bluesky 1.193Gi ± 2% 1.191Gi ± 1% ~ (p=0.574 n=8)
Tsc/sentry 3.357Gi ± 4% 3.626Gi ± 8% ~ (p=0.105 n=8)
Tsc/t3code-server 2.888Gi ± 3% 2.855Gi ± 3% -1.16% (p=0.038 n=8)
geomean 2.262Gi 2.310Gi +2.14%
│ before │ after │
│ heap-B │ heap-B vs base │
Tsc/bluesky 934.7Mi ± 0% 934.8Mi ± 0% ~ (p=0.645 n=8)
Tsc/sentry 2.609Gi ± 0% 2.608Gi ± 0% ~ (p=0.130 n=8)
Tsc/t3code-server 2.302Gi ± 0% 2.303Gi ± 0% ~ (p=0.878 n=8)
geomean 1.763Gi 1.763Gi -0.02%
Narrowing a property access like x.y walks the same long assignment
chains as narrowing an identifier, and was still visiting the syntax
of every assignment on the way. Such a reference can only be affected
by assignments to x itself or to access chains rooted at x.
Record the root identifier of access chain assignment targets as well,
marked so that identifier references still ignore them, and let walks
for access chains rooted at an identifier skip assignments rooted at
any other name.
--checkers 4, n=6:
│ before │ after │
│ sec/op │ sec/op vs base │
Tsc/bluesky 2.750 ± 6% 2.814 ± 4% ~ (p=0.084 n=6)
Tsc/sentry 11.28 ± 2% 11.34 ± 2% ~ (p=0.818 n=6)
Tsc/t3code-server 12.19 ± 9% 11.01 ± 4% -9.71% (p=0.002 n=6)
Tsc/vscode 12.93 ± 5% 12.90 ± 1% ~ (p=0.589 n=6)
geomean 8.363 8.203 -1.91%
│ before │ after │
│ rss-B │ rss-B vs base │
Tsc/bluesky 1.197Gi ± 2% 1.200Gi ± 2% ~ (p=0.818 n=6)
Tsc/sentry 3.527Gi ± 5% 3.512Gi ± 5% ~ (p=0.937 n=6)
Tsc/t3code-server 2.859Gi ± 3% 2.840Gi ± 2% ~ (p=0.818 n=6)
Tsc/vscode 4.826Gi ± 3% 4.969Gi ± 3% +2.96% (p=0.026 n=6)
geomean 2.763Gi 2.777Gi +0.51%
│ before │ after │
│ heap-B │ heap-B vs base │
Tsc/bluesky 934.7Mi ± 0% 934.7Mi ± 0% ~ (p=0.937 n=6)
Tsc/sentry 2.607Gi ± 0% 2.608Gi ± 0% ~ (p=0.937 n=6)
Tsc/t3code-server 2.303Gi ± 0% 2.303Gi ± 0% ~ (p=0.589 n=6)
Tsc/vscode 4.104Gi ± 0% 4.104Gi ± 0% ~ (p=0.589 n=6)
geomean 2.178Gi 2.178Gi +0.01%
|
TypeScript Bot (@typescript-bot) perf test this faster |
There was a problem hiding this comment.
🔵 Needs a closer look
The optimization bypasses core narrowing checks, and compiler-suite validation was unavailable to establish behavioral equivalence.
No open issues 🎉
What changed in this PR
Optimizes control-flow narrowing by skipping assignments that cannot affect the reference being checked.
Changes:
- Adds hashed assignment-target metadata to flow nodes.
- Populates that metadata during binding.
- Uses reference-root hashes to skip unrelated assignments during narrowing.
| File | Description |
|---|---|
tsc/internal/checker/flow.go |
Skips assignments using reference-root hashes. |
tsc/internal/binder/binder.go |
Records assignment-target metadata during binding. |
tsc/internal/ast/flow.go |
Defines target metadata and hashing helpers. |
Review effort: Balanced
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ryan Cavanaugh (RyanCavanaugh)
left a comment
There was a problem hiding this comment.
I was on the fence about this until I saw the t3code results... anyway the code is good but the comments aren't fully getting this through my brain without examples
| return h | ||
| } | ||
|
|
||
| // FlowAssignmentTargetHash summarizes which references an assignment flow node |
There was a problem hiding this comment.
This comment really needs code examples to illustrate what it's talking about
| return resultType | ||
| } | ||
|
|
||
| // getFlowReferenceRootHash returns the FlowIdentifierHash of the identifier a reference |
There was a problem hiding this comment.
Less so than the prior one, but same
See the two commits for more info.